Auto approve kubelet server certificate signing requests. #46884

jcbsmpsn · 2017-06-02T21:50:58Z

Release note:

Adds an approval work flow to the the certificate approver that will approve certificate signing requests from kubelets that meet all the criteria of kubelet server certificates.

liggitt · 2017-06-03T02:09:29Z

pkg/controller/certificates/approver/sarapprove.go

 	capi.UsageServerAuth,
 }

 func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {


This doesn't check it is from a node

It's missing a check like https://github.com/kubernetes/kubernetes/pull/46884/files#diff-05476437bf221fbe4cab028bf7ffbb74R172

mikedanese · 2017-06-07T22:42:08Z

pkg/controller/certificates/approver/sarapprove.go

@@ -187,14 +187,25 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert

 var kubeletServerUsages = []capi.KeyUsage{
 	capi.UsageKeyEncipherment,
-	capi.UsageDigitalSignature,
+	capi.UsageSigning,
 	capi.UsageServerAuth,
 }

 func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {


Feature flag this so it returns false if the kubelet server cert rotation feature is off.

mikedanese · 2017-06-07T22:42:51Z

You need this:

diff --git a/cluster/addons/rbac/kubelet-certificate-management.yaml b/cluster/addons/rbac/kubelet-certificate-management.yaml
index 4b4b3d7381..83f1187d57 100644
--- a/cluster/addons/rbac/kubelet-certificate-management.yaml
+++ b/cluster/addons/rbac/kubelet-certificate-management.yaml
@@ -57,5 +57,6 @@ rules:
   - "certificates.k8s.io"
   resources:
   - certificatesigningrequests/selfnodeclient
+  - certificatesigningrequests/selfnodeserver
   verbs:
   - "create"

mikedanese · 2017-06-08T08:23:58Z

pkg/controller/certificates/approver/sarapprove.go

 	capi.UsageServerAuth,
 }

 func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
-	if !hasExactUsages(csr, kubeletServerUsages) {
+	if utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {


Invert this. If feature not enabled return false.

mikedanese · 2017-06-09T16:32:11Z

/lgtm

mikedanese · 2017-06-09T16:35:18Z

/approve

liggitt · 2017-06-09T17:08:26Z

pkg/kubelet/kubelet.go

+			//
+			// Signing allows the certificate to be used to verify
+			// digital signatures used during TLS negotiation.
+			certificates.UsageSigning,


what key usage or ext key usage does this correspond to?

These are the same thing, I wish we had noticed that earlier.
https://github.com/cloudflare/cfssl/blob/437c6213e569dde639fd116272c27557fd38eaf9/config/config.go#L553

oh, digital signature...

"signing": x509.KeyUsageDigitalSignature, "digital signature": x509.KeyUsageDigitalSignature,

can we actually change the kubelet to use the usage that corresponds to the RFC? "signing" is pretty ambiguous

Changed to UsageDigitalSignature is both locations.

liggitt · 2017-06-09T17:16:52Z

pkg/controller/certificates/approver/sarapprove.go

@@ -187,14 +189,28 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert

 var kubeletServerUsages = []capi.KeyUsage{
 	capi.UsageKeyEncipherment,
-	capi.UsageDigitalSignature,
+	capi.UsageSigning,


I'd prefer this to keep the usage that matches the term used in the RFC

Changed back to UsageDigitalSignature.

mikedanese · 2017-06-09T19:55:55Z

/lgtm
/assign @derekwaynecarr
for kubelet approval

timstclair

/lgtm

timstclair · 2017-06-09T23:42:45Z

/approve

jcbsmpsn · 2017-06-10T01:23:57Z

/unassign @derekwaynecarr

liggitt · 2017-06-11T22:29:45Z

pkg/kubelet/kubelet.go

@@ -1124,16 +1124,25 @@ func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg
 		CertificateSigningRequestClient: certSigningRequestClient,
 		Template: &x509.CertificateRequest{
 			Subject: pkix.Name{
-				CommonName:   string(nodeName),
+				CommonName:   fmt.Sprintf("system:node:%s", nodeName),


I'd really like to see this change make 1.7. I'd prefer not to have a mix of CSR formats to recognize unnecessarily

Working on it.

mikedanese · 2017-06-13T01:25:11Z

@dchen1107 this fixes the alpha e2e tests and only touches alpha codepaths. Please add 1.7 milestone

mikedanese · 2017-06-15T20:37:47Z

We need this to fix broken e2es.

liggitt · 2017-06-15T20:41:45Z

pkg/controller/certificates/approver/sarapprove_test.go

 )

+func init() {
+	utilfeature.DefaultFeatureGate.Set(string(features.RotateKubeletServerCertificate) + "=true")


I wasn't expecting this... the approver is permission based... I only expected the kubelet rotation to be gated by the flag. Also, setting this one way in an init() function means it is always enabled when testing, right?

It is only enabled in this package test, yes.

liggitt · 2017-06-15T20:42:26Z

pkg/controller/certificates/approver/sarapprove.go

@@ -192,9 +194,23 @@ var kubeletServerUsages = []capi.KeyUsage{
 }

 func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {


I didn't expect the approver to be flag gated this way, only the kubelet function

if you want to gate it in the apiserver, gate adding isSelfNodeServerCert in recognizers(), and you can unit test isSelfNodeServerCert without messing with feature flag enablement in init()

I recommended to @jcbsmpsn that we do this. I really don't want to be tied to the behavior of isSelfNodeServer until we have thought through kubelets self reporting their SANs. What we currently allow is too weak, and this is an API.

ok, then move the gate to recognizers(), and remove the gate-twiddling from the init()

that sounds good to me.

dchen1107 · 2017-06-15T20:48:04Z

I am fine with kubelet change, which is trivial anyway. But I am not comfortable with the changes in pkg/controller/certificates/approver simply due to the lack of the knowledge from my side. I will leave @liggitt to make the final call.

liggitt · 2017-06-16T00:09:30Z

I0615 15:22:27.510] Verifying hack/make-rules/../../hack/verify-bazel.sh
I0615 15:22:33.070] --- /tmp/actual-file-768259978	2017-06-15 15:22:33.058595771 -0700
I0615 15:22:33.070] +++ /tmp/expected-file-304640865	2017-06-15 15:22:33.058595771 -0700
I0615 15:22:33.070] @@ -17,10 +17,8 @@
I0615 15:22:33.071]          "//pkg/apis/authorization/v1beta1:go_default_library",
I0615 15:22:33.071]          "//pkg/apis/certificates/v1beta1:go_default_library",
I0615 15:22:33.071]          "//pkg/client/clientset_generated/clientset/fake:go_default_library",
I0615 15:22:33.071] -        "//pkg/features:go_default_library",
I0615 15:22:33.071]          "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
I0615 15:22:33.072]          "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
I0615 15:22:33.072] -        "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
I0615 15:22:33.072]          "//vendor/k8s.io/client-go/testing:go_default_library",
I0615 15:22:33.072]      ],
I0615 15:22:33.072]  )
W0615 15:22:33.173] DRY-RUN: wrote "pkg/controller/certificates/approver/BUILD"
W0615 15:22:33.728] 
W0615 15:22:33.728] 1 BUILD files not up-to-date.
I0615 15:22:33.828] 
I0615 15:22:33.829] Run ./hack/update-bazel.sh
I0615 15:22:33.829] FAILED   hack/make-rules/../../hack/verify-bazel.sh	6s

mikedanese · 2017-06-16T17:25:18Z

/lgtm

liggitt · 2017-06-16T17:29:23Z

/lgtm

function is gated and off by default, and this helps fix the alpha features CI

k8s-github-robot · 2017-06-16T17:29:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcbsmpsn, liggitt, mikedanese, timstclair

Associated issue: 47208

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/OWNERS~~ [mikedanese]
~~pkg/controller/OWNERS~~ [mikedanese]
~~pkg/kubelet/OWNERS~~ [timstclair]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-16T18:33:58Z

Automatic merge from submit-queue (batch tested with PRs 46884, 47557)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2017

k8s-ci-robot assigned mikedanese Jun 2, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 2, 2017

liggitt reviewed Jun 3, 2017

View reviewed changes

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from fb0968a to 0aebbe3 Compare June 5, 2017 17:09

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 5, 2017

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 0aebbe3 to 1e4a7ef Compare June 6, 2017 18:45

mikedanese reviewed Jun 7, 2017

View reviewed changes

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 1e4a7ef to 993a783 Compare June 8, 2017 04:02

mikedanese reviewed Jun 8, 2017

View reviewed changes

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 993a783 to b34dc6e Compare June 9, 2017 14:44

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017

mikedanese mentioned this pull request Jun 9, 2017

alpha-features suite is failing to up the cluster #47208

Closed

kubernetes deleted a comment from k8s-github-robot Jun 9, 2017

liggitt reviewed Jun 9, 2017

View reviewed changes

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from b34dc6e to dacfede Compare June 9, 2017 17:56

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from dacfede to d2f9643 Compare June 9, 2017 18:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017

mikedanese assigned derekwaynecarr Jun 9, 2017

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from d2f9643 to 0b4f825 Compare June 9, 2017 20:39

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017

yujuhong assigned timstclair Jun 9, 2017

timstclair approved these changes Jun 9, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2017

k8s-ci-robot unassigned derekwaynecarr Jun 10, 2017

liggitt reviewed Jun 11, 2017

View reviewed changes

mikedanese changed the title ~~Auto approve kubelet certificate signing requests.~~ Auto approve kubelet server certificate signing requests. Jun 13, 2017

mikedanese added this to the v1.7 milestone Jun 15, 2017

liggitt reviewed Jun 15, 2017

View reviewed changes

dchen1107 assigned liggitt Jun 15, 2017

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 0b4f825 to e1b05e0 Compare June 15, 2017 22:14

Auto approve kubelet certificate signing requests.

334de1c

jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from e1b05e0 to 334de1c Compare June 16, 2017 15:47

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2017

k8s-github-robot merged commit aa7458a into kubernetes:master Jun 16, 2017

Auto approve kubelet server certificate signing requests. #46884

Auto approve kubelet server certificate signing requests. #46884

Conversation

jcbsmpsn commented Jun 2, 2017 • edited by liggitt

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jun 7, 2017 • edited

Choose a reason for hiding this comment

mikedanese commented Jun 9, 2017

mikedanese commented Jun 9, 2017

Choose a reason for hiding this comment

mikedanese Jun 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jun 9, 2017 • edited

timstclair left a comment

Choose a reason for hiding this comment

timstclair commented Jun 9, 2017

jcbsmpsn commented Jun 10, 2017

liggitt Jun 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jun 13, 2017

mikedanese commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Jun 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Jun 15, 2017

liggitt commented Jun 16, 2017

mikedanese commented Jun 16, 2017

liggitt commented Jun 16, 2017 • edited

k8s-github-robot commented Jun 16, 2017

k8s-github-robot commented Jun 16, 2017

jcbsmpsn commented Jun 2, 2017 •

edited by liggitt

mikedanese commented Jun 7, 2017 •

edited

mikedanese Jun 9, 2017 •

edited

mikedanese commented Jun 9, 2017 •

edited

liggitt Jun 11, 2017 •

edited

mikedanese Jun 15, 2017 •

edited

liggitt commented Jun 16, 2017 •

edited